consensus: fix integer overflow in ValidateCollateralRatio()#373
Closed
gto90 wants to merge 10 commits intofeature/digidollar-v1from
Closed
consensus: fix integer overflow in ValidateCollateralRatio()#373gto90 wants to merge 10 commits intofeature/digidollar-v1from
gto90 wants to merge 10 commits intofeature/digidollar-v1from
Conversation
There was a problem hiding this comment.
Pull request overview
This pull request addresses DGB-SEC-001, a critical integer overflow vulnerability in ValidateCollateralRatio() identified in security audit PR #367. The vulnerability occurs when multiplying collateralAmount * oraclePrice or ddAmount * requiredRatio, which can exceed INT64_MAX for large but legitimate values (e.g., 1M DGB at high prices).
Changes:
- Added overflow-safe multiplication using check-before-multiply pattern in two validation functions
- Implemented divide-first fallback when multiplication would overflow, consistent with existing pattern in
dca.cpp - Added comprehensive test coverage for overflow scenarios including normal, large, and MAX_MONEY values
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| src/consensus/digidollar_transaction_validation.cpp | Added overflow protection for collateralAmount * oraclePrice and ddAmount * requiredRatio multiplications using check-before-multiply pattern |
| src/digidollar/validation.cpp | Added overflow protection for dgbLocked * ctx.oraclePriceMicroUSD and division-by-zero guard for ddMinted > 0 |
| src/test/digidollar_transaction_tests.cpp | Added test case test_collateral_ratio_overflow_protection covering normal values, large collateral, MAX_MONEY, and edge cases |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
8 tasks
Use $$ deferred evaluation for CFLAGS/CPPFLAGS assignments so OS-specific flags (cflags_linux, cppflags_linux) appended by funcs.mk after set_vars are included. Route -fPIC and -D_GNU_SOURCE through proper cflags/cppflags variables instead of bare config_opts to avoid OpenSSL's "Mixing make variables" rejection.
The depends system never defines $(package)_arflags, so ARFLAGS=$($(package)_arflags) in config_env exported an empty ARFLAGS="" to the environment. When CFLAGS/CPPFLAGS are passed as VAR=value assignments (not positional flags), OpenSSL 1.1.1w's Configure sets $anyuseradd=false and falls back to reading env vars. The empty ARFLAGS overrides the target default "r" from Configurations/00-base-templates.conf, producing a Makefile with ARFLAGS= (empty). This causes "ar: two different operation options specified" on Linux and "ar: illegal option -- /" on macOS during build_libs, as ar interprets the archive path as flags. Fix: remove ARFLAGS from config_env entirely, letting Configure use its target default ARFLAGS="r". Verified locally: - Linux x86_64: ar r apps/libapps.a ... (ARFLAGS=r in Makefile) - macOS ARM64: ar r apps/libapps.a ... (ARFLAGS=r in Makefile) - Full build_libs completes successfully on darwin64-arm64-cc
Replace BOOST_CHECK with BOOST_CHECK_MESSAGE in 5 mint validation tests that fail on CI but pass locally. Diagnostic output captures the exact reject reason, script sizes, collateral amounts, and script type identification to help debug the CI-specific failure.
Replace ClearFreeze() with ClearHistory() in the validation test fixture. ClearFreeze() only clears freeze flags, but UpdateState() (called during ValidateDigiDollarTransaction) recalculates volatility from stale price history left by earlier test suites (e.g., digidollar_health_tests) and re-sets the freeze. This caused all "valid mint" tests to fail with "minting-frozen-volatility" on CI where the test suite ordering (linker-dependent) places health_tests before validation_tests. ClearHistory() resets price history, volatility state, and all freeze flags, ensuring each test starts from a clean state.
Previous CI re-run used old code without the fix. Canceling that and triggering fresh run.
Trigger fresh CI run with the cross-suite state pollution fix.
Keep our diagnostic BOOST_CHECK_MESSAGE over base branch's BOOST_TEST_MESSAGE for better CI failure reporting.
The merge with feature/digidollar-v1 silently dropped critical code: 1. DD amount double-counting prevention in ValidateMintTransaction() The OP_RETURN and DD token output both encode ddAmount. Without the consistency check, totalDD was counted twice (e.g. 10000 + 10000 = 20000), requiring 2x the collateral and failing all valid mint tests. 2. Missing #include <digidollar/health.h> 3. Improved log format for insufficient-collateral diagnostic Also adopts the base branch's test file which includes: - DD OP_RETURN outputs required for T1-04b NUMS verification - Proper NUMS key usage via GetCollateralNUMSKey() - ClearHistory() fix for cross-suite volatility state pollution
|
Closing as duplicate — already fixed with a better approach on Why this is superseded:This PR uses a "divide first to avoid overflow" pattern in
The vulnerability was real (SEC-001) and the analysis was correct — we just fixed it differently during the overnight red team session. Thanks @gto90 for identifying the overflow vectors. |
The test at line 4133 created two separate temporary XOnlyPubKey objects — one for .begin() and another for .end() — producing iterators into different memory regions. This is undefined behavior that manifests as std::length_error on some platforms. Fix: create a named XOnlyPubKey variable and use the existing MakeP2TR() helper instead of manual CScript construction.
gto90
added a commit
that referenced
this pull request
Feb 14, 2026
On forked repositories, pull_request events only reliably trigger CI for PRs targeting the default branch (develop). PRs targeting non-default branches like feature/digidollar-v1 silently fail to trigger workflows — confirmed across 7 previous PRs (#373-378, #369) that all had zero CI runs. Add feature/** and fix/** to push triggers so CI runs directly on push. Add workflow_dispatch as a manual fallback from the Actions tab.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation
Fixes DGB-SEC-001 from PR #367 security audit.
ValidateCollateralRatio()performscollateralAmount * oraclePricewhich overflowsint64_tfor large but legitimate values (e.g. 1M DGB at $10/DGB = 10^20 > INT64_MAX 9.2x10^18). The same overflow exists forddAmount * requiredRatio.Description
src/consensus/digidollar_transaction_validation.cpp:dca.cpp:60-70)INT64_MAX / multipliersrc/digidollar/validation.cpp:dgbLocked * ctx.oraclePriceMicroUSDddMinted > 0guard to prevent division by zero in ratio calculationsrc/test/digidollar_transaction_tests.cpp:test_collateral_ratio_overflow_protectiontest caseTesting
i2p_tests(unrelated)Refs: PR #367 (DGB-SEC-001)